Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pull Kyber division fixes from PQ-Crystals into main #1649

Merged
merged 6 commits into from
Jan 8, 2024
Merged

Pull Kyber division fixes from PQ-Crystals into main #1649

merged 6 commits into from
Jan 8, 2024

Conversation

praveksharma
Copy link
Member

@praveksharma praveksharma commented Jan 2, 2024 •
edited
Loading

Pull recent fixes made to Kyber from PQ-Crystals into main branch and patches Kyber aarch64 implementation.

Fixes #1645 (see also #1652).

  • [No] Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • [No] Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider and OQS-OpenSSH will also need to be ready for review and merge by the time this is merged.)

@praveksharma praveksharma changed the title Pull Kyber division fixes from PQ-Crystals Pull Kyber division fixes from PQ-Crystals into main Jan 2, 2024
bhess
bhess reviewed Jan 3, 2024
View reviewed changes
Copy link
Member

@bhess bhess left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these updates @praveksharma & @SWilson4.

scripts/copy_from_upstream/copy_from_upstream.yml Show resolved Hide resolved
@dstebila
Copy link
Member

dstebila commented Jan 4, 2024

I checked with @cryptojedi and he says that after this there shouldn't be any operations of the form /KYBER_Q left in the source code -- we could consider a CI test that grep's for those. He also writes that one could compile with -Os and then check if the output has an DIV instructions.

@praveksharma praveksharma mentioned this pull request Jan 5, 2024
Merged
1 task
@SWilson4 SWilson4 marked this pull request as ready for review January 5, 2024 18:05
@SWilson4 SWilson4 requested a review from bhess January 5, 2024 18:05
@dstebila dstebila added this to the 0.10.0 milestone Jan 5, 2024
@SWilson4 SWilson4 mentioned this pull request Jan 5, 2024
Open
baentsch
baentsch approved these changes Jan 6, 2024
View reviewed changes
Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM - just some "musing" on documentation as per comment.

docs/algorithms/kem/kyber.md
- **Implementation license (SPDX-Identifier)**: CC0-1.0 or Apache-2.0
- **Optimized Implementation sources**: https://github.com/pq-crystals/kyber/commit/dda29cc63af721981ee2c831cf00822e69be3220 with copy_from_upstream patches
- **Optimized Implementation sources**: https://github.com/pq-crystals/kyber/commit/b628ba78711bc28327dc7d2d5c074a00f061884e with copy_from_upstream patches
- **oldpqclean-aarch64**:<a name="oldpqclean-aarch64"></a>
- **Source**: https://github.com/PQClean/PQClean/commit/8e220a87308154d48fdfac40abbb191ac7fce06a with copy_from_upstream patches
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better solution for this? There's apparently no trace (at least in the documentation) as to whether the (aarch64) code version that liboqs runs does or does not contain the "DIV" fix. Code review shows it does, but a more simple inspection by checking these alg documentation files does not. It's "hidden" within the statement (and a changed file contents of) "copy_from_upstream patches". Maybe worth while adding a hash of those patches here at some time?

@dstebila dstebila merged commit c2c969c into main Jan 8, 2024
56 checks passed
@SWilson4 SWilson4 deleted the ps-kyber-fix branch January 18, 2024 21:33
@BrewTestBot BrewTestBot mentioned this pull request Mar 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SWilson4 SWilson4 SWilson4 left review comments

@baentsch baentsch baentsch approved these changes

@dstebila dstebila dstebila approved these changes

@bhess bhess bhess approved these changes

Assignees

@praveksharma praveksharma

Labels
None yet
Projects
None yet
Milestone
0.10.0
Development

Successfully merging this pull request may close these issues.

Further Kyber division fixes
5 participants
@praveksharma @dstebila @bhess @SWilson4 @baentsch